Add integration tests for neural query #36

jmazanec15 · 2022-10-28T00:06:53Z

Description

Adds a series of integration tests for neural query type. Adds shared functionality to base class as well as a utility class.

Issues Resolved

#14

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

navneet1v · 2022-10-28T18:05:33Z

src/test/java/org/opensearch/neuralsearch/common/BaseNeuralSearchIT.java

 import org.opensearch.common.xcontent.XContentFactory;
 import org.opensearch.common.xcontent.XContentHelper;
 import org.opensearch.common.xcontent.XContentType;
+import org.opensearch.index.query.QueryBuilder;
+import org.opensearch.rest.RestStatus;


I know this is not the change which you have done, but can you change the base class of BaseNeuralSearchIT from OpenSearchRestTestCase to OpenSearchSecureRestTestCase. This will allow the secure clusters to be used for testing

Yes, I can update.

navneet1v · 2022-10-28T18:06:13Z

src/test/java/org/opensearch/neuralsearch/common/BaseNeuralSearchIT.java

+     * @return number of documents indexed to that index
+     */
+    @SneakyThrows
+    public int getDocCount(String indexName) {


why public? can't we make it protected?

Good catch, will update.

navneet1v · 2022-10-28T18:08:29Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+        modelId.compareAndSet(null, prepareModel());
+    }
+
+    private void maybeInitializeIndex(String indexName) throws IOException {


better name for function can be : initializeIndexIfNotExist

Will update

navneet1v · 2022-10-28T18:09:27Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+        modelId.compareAndSet(null, prepareModel());
+    }
+
+    private void maybeInitializeIndex(String indexName) throws IOException {


[nit-pick] move this private function after all the public and protected functions.

navneet1v · 2022-10-28T18:12:26Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+    }
+
+    @SneakyThrows
+    public void testBasicQuery() {


suggestion: as the knowledge of tests will fade as the time passes, lets add the query and response objects in the java doc so that we know what to expect for all the tests present in this class.

Will update

Each response will be similar. I am going to just provide query.

navneet1v · 2022-10-28T18:14:05Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+    }
+
+    @SneakyThrows
+    private String prepareModel() {


should we move this to BaseNeuralSearchIT class?

navneet1v · 2022-10-28T18:17:14Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+    private void prepareKnnIndex(String indexName, List<KNNFieldConfig> knnFieldConfigs) {
+        createIndexWithConfiguration(indexName, buildIndexConfiguration(knnFieldConfigs), "");
+    }
+
+    @SneakyThrows
+    private String buildIndexConfiguration(List<KNNFieldConfig> knnFieldConfigs) {
+        XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()


move all of these to BaseNeuralSearchIT, as we have search, indexCreation and addKNNDoc all functions there. Plus I believe these can be reused across different ITs.

Good point. Will move. Will leave initializeIndexIfNotExist because this is more specific to this particular test suite.

navneet1v · 2022-10-28T18:19:06Z

src/test/java/org/opensearch/neuralsearch/common/BaseNeuralSearchIT.java

+    public float[] runInference(String modelId, String queryText) {
+        Response inferenceResponse = makeRequest(
+            client(),
+            "POST",
+            String.format(LOCALE, "/_plugins/_ml/_predict/text_embedding/%s", modelId),
+            null,
+            toHttpEntity(String.format(LOCALE, "{\"text_docs\": [\"%s\"],\"target_response\": [\"sentence_embedding\"]}", queryText)),
+            ImmutableList.of(new BasicHeader(HttpHeaders.USER_AGENT, "Kibana"))
+        );
+
+        Map<String, Object> inferenceResJson = XContentHelper.convertToMap(
+            XContentFactory.xContent(XContentType.JSON),
+            EntityUtils.toString(inferenceResponse.getEntity()),
+            false
+        );
+
+        Object inference_results = inferenceResJson.get("inference_results");
+        assertTrue(inference_results instanceof List);
+        List<Object> inferenceResultsAsMap = (List<Object>) inference_results;
+        assertEquals(1, inferenceResultsAsMap.size());
+        Map<String, Object> result = (Map<String, Object>) inferenceResultsAsMap.get(0);
+        List<Object> output = (List<Object>) result.get("output");
+        assertEquals(1, output.size());
+        Map<String, Object> map = (Map<String, Object>) output.get(0);
+        List<Float> data = ((List<Double>) map.get("data")).stream().map(Double::floatValue).collect(Collectors.toList());
+        return vectorAsListToArray(data);
+    }


What was the reason for not using the MLCommonsClient here and directly use the inference API?

No, not really any reason. I can switch to it.

Ah actually, I think we need to use rest because from these tests we only have access to RestClient. MLCommonsClientAccessor requires a MachineLearningNodeClient which requires a Client which is internal to the cluster.

jmazanec15 · 2022-10-28T22:23:13Z

src/test/resources/processor/UploadModelRequestBody.json

    "framework_type": "sentence_transformers",
    "all_config": "{\"architectures\":[\"BertModel\"],\"max_position_embeddings\":512,\"model_type\":\"bert\",\"num_attention_heads\":12,\"num_hidden_layers\":6}"
  },
-  "url": "https://github.com/opensearch-project/ml-commons/blob/2.x/ml-algorithms/src/test/resources/org/opensearch/ml/engine/algorithms/text_embedding/all-MiniLM-L6-v2_torchscript_sentence-transformer.zip?raw=true"
+  "url": "https://github.com/jmazanec15/k-NN-1/blob/model/traced_small_model.zip?raw=true"


Temporary: Will update URL to ml-commons once uploaded

@jmazanec15 , new model updated in ml-commons, you can use this one https://github.com/opensearch-project/ml-commons/blob/2.x/ml-algorithms/src/test/resources/org/opensearch/ml/engine/algorithms/text_embedding/traced_small_model.zip?raw=true

Cool just switched it.

jmazanec15 · 2022-10-31T16:44:52Z

Increasing the heap size of a node in the test cluster to 1GB. Default is 512mb which may be too small for ml - https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L845

vamshin · 2022-10-31T18:10:45Z

src/test/java/org/opensearch/neuralsearch/common/BaseNeuralSearchIT.java


 import com.google.common.collect.ImmutableList;

-public abstract class BaseNeuralSearchIT extends OpenSearchRestTestCase {
+public abstract class BaseNeuralSearchIT extends OpenSearchSecureRestTestCase {

    private static final Locale LOCALE = Locale.getDefault();


Not a issue for this PR. Looks like Locale.getDefault() seem to have issue with Windows shall we use Locale.ROOT? Take it in other PR

Let use Locale.ROOT.

I think you have not rebased the code, this statement has been updated.

Will rebase

vamshin · 2022-10-31T18:16:57Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+     *    }
+     */
+    @SneakyThrows
+    public void testRescoreQuery() {


Should we add some function to rescore like weight?

I didnt think it was necessary given that weight will be outside of the neural query

vamshin · 2022-10-31T18:17:49Z

src/test/java/org/opensearch/neuralsearch/plugin/query/NeuralQueryIT.java

+    /**
+     * Tests bool should query:


How about bool query one with neural and one with BM25?

Adds a series of integration tests for neural query type. Adds shared functionality to base class as well as a utility class. Signed-off-by: John Mazanec <[email protected]>

Signed-off-by: John Mazanec <[email protected]>

vamshin

LGTM! Thanks

Adds a series of integration tests for neural query type. Adds shared functionality to base class as well as a utility class. Increase test cluster heap to 1 GB. Signed-off-by: John Mazanec <[email protected]> (cherry picked from commit e30285b)

jmazanec15 requested a review from a team October 28, 2022 00:06

jmazanec15 added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Oct 28, 2022

jmazanec15 requested review from navneet1v and zane-neo October 28, 2022 01:20

navneet1v reviewed Oct 28, 2022

View reviewed changes

jmazanec15 commented Oct 28, 2022

View reviewed changes

jmazanec15 requested a review from navneet1v October 28, 2022 23:13

zane-neo approved these changes Oct 29, 2022

View reviewed changes

jmazanec15 requested a review from ylwu-amzn October 31, 2022 16:07

vamshin reviewed Oct 31, 2022

View reviewed changes

jmazanec15 added 8 commits October 31, 2022 14:19

Add integration tests for neural query

70ac25c

Adds a series of integration tests for neural query type. Adds shared functionality to base class as well as a utility class. Signed-off-by: John Mazanec <[email protected]>

Update model to smaller version

f8e0879

Signed-off-by: John Mazanec <[email protected]>

Address feedback from PR first round

d6d38b1

Signed-off-by: John Mazanec <[email protected]>

Fix model download link

50ed47e

Signed-off-by: John Mazanec <[email protected]>

Increase test cluster to 1 GB

960c289

Signed-off-by: John Mazanec <[email protected]>

Fix heap size settings

c43ce80

Signed-off-by: John Mazanec <[email protected]>

Add comment on build file

5b1322e

Signed-off-by: John Mazanec <[email protected]>

Add bms25+neural query

52531c1

Signed-off-by: John Mazanec <[email protected]>

jmazanec15 force-pushed the query-integ-tests branch from 5b81d70 to 52531c1 Compare October 31, 2022 21:39

jmazanec15 requested a review from vamshin October 31, 2022 21:40

vamshin approved these changes Oct 31, 2022

View reviewed changes

jmazanec15 merged commit e30285b into opensearch-project:main Oct 31, 2022

opensearch-trigger-bot bot mentioned this pull request Oct 31, 2022

[Backport 2.x] Add integration tests for neural query #47

Merged

jmazanec15 added the Infrastructure label Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration tests for neural query #36

Add integration tests for neural query #36

jmazanec15 commented Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

navneet1v Oct 28, 2022

jmazanec15 Oct 28, 2022

jmazanec15 Oct 28, 2022

jmazanec15 Oct 28, 2022

ylwu-amzn Oct 29, 2022 •

edited

Loading

jmazanec15 Oct 31, 2022

jmazanec15 commented Oct 31, 2022

vamshin Oct 31, 2022

navneet1v Oct 31, 2022

jmazanec15 Oct 31, 2022

vamshin Oct 31, 2022

jmazanec15 Oct 31, 2022

vamshin Oct 31, 2022

jmazanec15 Oct 31, 2022

vamshin left a comment

Add integration tests for neural query #36

Add integration tests for neural query #36

Conversation

jmazanec15 commented Oct 28, 2022

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylwu-amzn Oct 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vamshin left a comment

Choose a reason for hiding this comment

ylwu-amzn Oct 29, 2022 •

edited

Loading